AI Code Review

Hello team!

This pull request introduces some really exciting changes around autonomous path management and a new, more sophisticated launch system for the shooter. It's great to see new architectural patterns being implemented, especially with the LaunchState and LaunchCalculator!

Positive Highlights

• Command-Based Architecture & Superstructure Pattern: The introduction of LaunchState and LaunchCalculator is an excellent step towards implementing the Superstructure pattern. It centralizes complex logic and provides a clear interface for other parts of the robot to request launch parameters.
• Type-Safe Units: I love seeing WPILib's Units system consistently used in LaunchRequest, LaunchCalculator, and ShooterConstants. This greatly reduces the chance of unit conversion errors.
• Modular Design: The refactoring of LaunchRequestBuilder and its implementations (MappedLaunchRequestBuilder, ParabolicLaunchRequestBuilder) to receive calculated parameters (distance, target angles) rather than doing the pose prediction themselves is a fantastic improvement in modularity and separation of concerns.

Suggestions

Code Quality & Java Standards

1. LaunchCalculator.java - Magic Number:
   • Suggestion: In LaunchCalculator.java, the loopPeriodSecs (0.02) is a magic number. Consider defining this as a constant in a RobotConstants.java file or passing it into the constructor.
   • Why it matters: Using named constants makes the code easier to understand and maintain, especially if the loop period ever changes.
2. LaunchCalculator.java - Clarity in Pose Prediction:
   • Suggestion: The line estimatedRobotPose.exp(...) modifies estimatedRobotPose directly. While correct, it can be slightly less clear that this is a predicted pose. Consider creating a new variable, e.g., Pose2d predictedRobotPose = estimatedRobotPose.exp(...), for better readability.
3. LaunchCalculator.java - Naming in getDriveAngle:
   • Suggestion: The variable hubAngle in getDriveAngle might be a bit misleading as it's an intermediate calculation, not directly representing the "hub." Perhaps angleOffset or relativeAngleAdjustment would be clearer names.
4. DriveState.java - Exception Handling:
   • Suggestion: In grabVisionEstimateList, the catch (Exception e) is very broad. It might be helpful to log the full stack trace (e.printStackTrace()) for better debugging, or even consider catching more specific exceptions if possible.
   • Why it matters: Catching Exception can hide unexpected issues. Logging the stack trace helps pinpoint the exact problem if one occurs.
5. IndexerPreferences.java - Naming (if applicable):
   • Question: Assuming IntakePreferences.java was renamed to IndexerPreferences.java, the names indexSpeed and indexerPercent are both DoublePreference. If indexerPercent is specifically for open-loop control without PID, perhaps indexerDutyCyclePercent or indexerOpenLoopPercent would make its purpose even clearer.
6. LaunchRequest.java - Hood Angle Conversion:
   • Suggestion: The conversion for launchHoodTarget in the LaunchRequest constructor:

     this.launchHoodTarget = Rotations.of(launchHoodTarget.in(Degrees) * ShooterConstants.ROTATIONS_PER_LAUNCH_DEGREE.in(Rotations));

     This looks like it might be converting Degrees to Rotations by multiplying by 1. ShooterConstants.ROTATIONS_PER_LAUNCH_DEGREE is currently Rotations.of(1). If ROTATIONS_PER_LAUNCH_DEGREE is intended to be a conversion factor (e.g., X rotations per degree of hood movement), it should be defined as a double or a Unit<Dimensionless> representing Rotations.per(Degrees). Please double-check this conversion and the constant's definition in ShooterConstants.

FRC/WPILib Best Practices

1. RobotContainer.java - AutoBuilder Configuration:
   • Suggestion: PathPlannerLib 2024+ (and 2025.0 in your settings.json) deprecates RobotConfig.fromGUISettings(). It might be helpful to update the AutoBuilder.configure call in DrivetrainSubsystem.java to use AutoBuilder.configureHolonomic() directly for the latest best practices.
   • Why it matters: Staying current with WPILib/PathPlannerLib APIs helps with future compatibility and access to new features.
2. LaunchState.java - Request Refreshing:
   • Suggestion: The if (currentRequest.getTimestamp() + 0.02 < Utils.getCurrentTimeSeconds()) refreshRequest(); logic in getLaunchRequest() ensures the request is fresh when it's accessed. Consider moving the refreshRequest() call to robotPeriodic() in Robot.java (gated by isActivated()) to ensure the LaunchRequest is continuously updated every loop period, regardless of whether it's immediately accessed.
   • Why it matters: This ensures the launch parameters are always up-to-date with the robot's current pose and velocity, even if a command isn't actively polling getLaunchRequest() in every single loop.
3. ShooterSubsystem.java - Consistency in Constants:
   • Suggestion: In atHoodSetpoint(), the method uses IntakeConstants.ALLOWABLE_EXTENSION_ERROR. It would be more robust to define a dedicated ALLOWABLE_HOOD_ERROR constant in ShooterConstants.java to avoid coupling the shooter's tolerance to the intake's.
   • Why it matters: Different mechanisms often require different tolerances. Using a specific constant for the shooter hood prevents unexpected behavior if the intake's tolerance is changed.
4. ShooterSubsystem.java - Naming for PID vs. NoPID Commands:
   • Suggestion: You have launchLemonsCommand() and launchLemonsCommandNoPID(). For clarity, consider renaming launchLemonsCommand() to launchLemonsCommandWithPID() or perhaps prepareForLaunchCommand() if the PID/non-PID choice is handled internally via a preference.
   • Why it matters: Consistent and descriptive naming makes it easier to understand the intent and behavior of commands.

Custom Rules & Best Practices

1. ParabolicLaunchRequestBuilder.java - Robustness of do-while Loop:
   • Suggestion: The do-while loop for calculating a, b, vertex, theta, motorAngle by decreasing slope by 0.05 is a heuristic.
     • The condition !(motorAngle.in(Degrees) < ShooterConstants.MIN_HOOD_ANGLE.in(Degrees)) means the loop continues as long as the motor angle is not less than the minimum. This seems inverted. It should likely continue as long as motorAngle is greater than MIN_HOOD_ANGLE (and other conditions are met).
     • Consider adding a maximum iteration count to prevent an infinite loop if a valid solution isn't found within a reasonable range of slopes. If no valid solution is found, createLaunchRequest should probably return null and LaunchState should handle that case.
   • Why it matters: Heuristic loops can sometimes lead to unexpected behavior or infinite loops if the conditions aren't carefully managed. Ensuring termination and handling failure cases is crucial for robot reliability.
2. ParabolicLaunchRequestBuilder.java - Magic Number (Gravity):
   • Suggestion: The constant 9.8 for gravity in the LinearVelocity calculation should be defined as a constant (e.g., Constants.GRAVITY_MPS_SQUARED) for clarity and consistency.
   • Why it matters: Named constants improve readability and make it easier to find and update values.
3. ShooterConstants.java - Verify Physical Constants:
   • Critical Suggestion: Please verify the values for FLYWHEEL_RADIUS = Inch.of(1) and ROTATIONS_PER_LAUNCH_DEGREE = Rotations.of(1).
     • A 1-inch radius flywheel seems extremely small and would lead to very high calculated angular velocities.
     • ROTATIONS_PER_LAUNCH_DEGREE (currently Rotations.of(1)) should represent the gear ratio or conversion factor from degrees of hood angle to motor encoder rotations. This is very likely incorrect as it stands and will cause incorrect hood positioning.
   • Why it matters: Incorrect physical constants will lead to inaccurate launch calculations and the shooter not performing as intended. These are critical values!
4. LaunchCalculator, LaunchState, LaunchRequest - Javadoc:
   • Suggestion: These new classes are fundamental to the shooter's new logic. Adding Javadoc to their public methods, constructors, and the classes themselves would be very beneficial for future maintainers.
   • Why it matters: Good documentation is essential for understanding complex systems, especially in a learning environment like FRC.

Questions

• simgui-ds.json: What was the specific purpose of adding Keyboard0 to robotJoysticks? Is this for a specific simulation setup or debugging?
• PathPlanner Autos (resetOdom): For the autos where resetOdom was changed to false (e.g., Plus Auto.auto, Rotate and Move Auto.auto, Square Auto.auto), is this intentional for testing specific path chaining scenarios? Just checking if it's a deliberate design choice.
• CameraConstants.java - Tilted Camera Use: You've added photonCameraTransform_Front_TILTED. Is there a mechanism or a plan to switch between the static and tilted camera transforms dynamically, or is the tilted one for future development?

This is a fantastic amount of work, especially with the new launch calculation logic! Keep up the great work, and I'm looking forward to seeing these improvements in action.

This review was automatically generated by AI. Please use your judgment and feel free to discuss any suggestions!

✓ Build successful and code formatting check passed!